fix(desktop): localize expected errors - #4457
Conversation
53b08a5 to
b6ef648
Compare
|
Converting to draft: blocked on the copy-gate/architecture-ratchet collision described in #2672 (comment) — #4493 resolves the checker policy; this branch then rebases (and splits the global error mapper per review). Will mark ready once green. |
adf4f4a to
6b28e30
Compare
…cture ratchet (#4493) Two repository gates deadlocked. The locale policy (#2672) moves user-visible copy out of business files into src/renderer/locales/*-copy.ts catalogs, which adds an import edge; the renderer architecture ratchet forbids legacy files from growing their dependency count and rejects new AppShell-closure entries. #4457 failed CI on exactly that collision, and no placement of a catalog could satisfy both gates. Admit one dependency class instead of loosening the ratchet: a validated copy catalog, recognized structurally and re-verified on every run, never grandfathered. It must live under src/renderer/locales/*-copy.ts, carry the UiCatalog marker from @maka/core/ui-locale, have zero tracked capabilities by the checker's own metrics, and import bare package specifiers only, so it can never become a tunnel to renderer implementation. Admitted edges are excluded from the dependency-count ratchets and closure admission, and the ledger drops 26 budget entries that only existed for catalog imports. Root-entry files get no discount, and a catalog that grows a hook or a relative import loses admission immediately. The environment-capability predicate now counts identifiers only in value-reference positions, so a copy key named history or a parameter named location no longer reads as a browser global, and type-only imports and exports are not counted as runtime dependencies. Adversarial fixtures cover hook smuggling, implementation-import smuggling, a missing marker, dynamic imports and an unrelated dependency added beside a valid catalog. Part of #2672. Generated-by: Codex
13b0d58 to
ad06382
Compare
9050c08 to
47537fd
Compare
3c22d3a to
5925b9f
Compare
|
Rebased onto current Verified on the rebased head: Desktop suite 2311/0, four typechecks 0 errors (the |
364a646 to
df0f399
Compare
|
@Astro-Han Rebased and CI passed now |
6e6bef9 to
5625712
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the complete 50-file merge-base diff at 5625712. The earlier Plan false-success and missing-pending defects are fixed. I found no reproduced P0–P2 production defect; one nonblocking P3 regression-test gap is detailed inline.
This changes expected-error transport and presentation, not just copy: Plan preserves structured Host failures over IPC; Work Board, attachments and collaboration map stable codes to locale catalogs; unexpected errors use redacted diagnostics; WorkHub copy is separated; WebSearch settlement retains a nonempty error message. I followed producer/transport/presenter paths and verified all declared Plan error codes through the registered handlers, including an actual missing-proposal storage failure. The deferred React hook test also confirms pending stays true until the bridge settles.
A clean lockfile install, repository dependency patches, full build:test and 158 focused compiled tests passed. The independent producer/IPC/pending probes passed separately. Hosted test and package are successful on this head. No real Electron UI, native-platform acceptance or full-repository test run is claimed.
This head is not merge-ready: GitHub reports CONFLICTING/DIRTY, and merge-tree against current main 2310035 independently reports conflicts in renderer-architecture.json and workhub-surface.tsx. Resolve those and validate the resulting new head; the current review cannot certify that future merge result.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
5625712 to
ae0e0cf
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed the complete rebased change at ae0e0cf. The previous P3 regression-test gap is fixed; no new substantiated P0–P3 finding.
The new test exercises all five registered Plan IPC handlers with typed Host failures, checks the preserved error code and asserts that no mode change is emitted. Disabling the actual production conversion now makes that test fail, unlike the previous head. The unexpected-error rethrow and React pending regressions also remain covered. I checked the rebase adaptations for current WorkHub resume outcomes/clarifications, locale catalogs, fallback-title callers and the protocol compatibility acknowledgment.
On this isolated head, clean install plus repository dependency patches, full build:test, 167 focused compiled tests and 101 renderer-architecture tests all passed; the actual architecture check passed too. Real Electron visual behavior, the full Desktop suite and native-platform acceptance were not run.
The previous main-conflict caveat is now resolved: GitHub reports MERGEABLE/CLEAN, hosted test/package are successful, and a local merge-tree against current main 2310035 is clean. This COMMENT records source and test results; it is not a human review or merge authorization.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Squash of the stacked increment, rebased onto fix/desktop-error-codes (apache#4457) after its rewrite onto main: typed AttachmentIngestBlockedError with the attachment_blocked envelope, ExpectedOperationError across the preload and session-settings surface, typed session-configuration transition errors in the Runtime, and the sessionSettingsActions updateFailures catalog per locale. localizedShellErrorMessage no longer parses any token. Generated-by: Claude Code Generated-by: OpenCode
Squash of the stacked increment, rebased onto fix/desktop-error-codes (apache#4457) after its rewrite onto main: typed AttachmentIngestBlockedError with the attachment_blocked envelope, ExpectedOperationError across the preload and session-settings surface, typed session-configuration transition errors in the Runtime, and the sessionSettingsActions updateFailures catalog per locale. localizedShellErrorMessage no longer parses any token. Generated-by: Claude Code Generated-by: OpenCode
|
@Astro-Han Rebased. PR is ready to be merged |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the localization work. Additional independent review of ae0e0cff46611dcdf26472b95f0455c0f8d4f108 found one remaining attachment acceptance gap in the normal new-task send path. The IPC envelope behavior was independently exercised in Electron; the finding concerns a renderer preflight that runs before that boundary.
AI disclosure: Codex agents performed this review, and the coordinating agent checked the production call path and reran the preflight/presenter counterexample. This is not an independent human review.
中文摘要
重新审查发现新建任务发送附件时仍有一条未覆盖的正常路径。Electron IPC envelope 已独立实测;这里的问题发生在进入 IPC 前的 renderer 预检。主审已核对真实发送调用链,并重跑预检与错误映射反例。这是自动化评审,不冒充人工验收。
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the updates. A fresh, multi-reviewer pass at ae0e0cff46611dcdf26472b95f0455c0f8d4f108 found one additional, optional ownership simplification below. The previously reported new-task attachment preflight issue remains unchanged. This review was AI-assisted and cross-checked by the coordinating AI reviewer; it does not represent completed human visual acceptance.
中文
谢谢更新。本轮独立深审新增一条可选的职责归属精简意见;此前新任务附件预检的问题仍未变化。本次由 AI 协助审查并由主审 AI 交叉验证,不代表已经完成人工视觉验收。
6474464 to
f1735c5
Compare
f1735c5 to
9e1759d
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks. A focused size/complexity follow-up is attached inline. This is AI-assisted source review, cross-checked by the coordinating reviewer; no new full-suite run.
中文
感谢更新,行内补充一条规模与复杂度复核意见。本次为 AI 辅助源码审查,经主审交叉核对,未重跑全仓测试。
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the localization work and the production-path fixes. The combined review/simplification audit found three additional P3 opportunities to reduce new contracts; they are inline. The existing future-only Session-control token comment still applies.
One further optional consolidation spans several files: reportUnexpectedError and unexpectedErrorFallback merely forward to the new core diagnostic helpers. Diagnostic callers can use those core helpers directly, removing the forwarding API and its dependency edges; keep ExpectedOperationError, which has actual WorkBoard/WorkHub consumers.
The Plan IPC envelope itself, domain-owned catalogs, attachment preflight validation, and the real pending/IPC regression coverage have current obligations and should remain.
AI-assisted review by Codex; checked against the production producers and consumers.
中文
本轮新增三项 P3 简化意见见行内,之前未来 Session-control token 的意见仍适用。另可让诊断调用方直接使用 core helper,删除两个纯转发 API;ExpectedOperationError 有真实消费者,应保留。Plan IPC envelope、领域 catalog、附件预检及真实 pending/IPC 回归保护都有当前义务,不建议整片删除。
9e1759d to
ba95e08
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the reduction. Rechecked ba95e0823, separating the rebase from the new fix commit: the future-only Session token mechanism is gone, Web Search retains its existing required-message contract, the client exception is back at its client owner, and attachment aliases/copy are consolidated. The extra fix commit removes more than it adds (+66/-148). No P0–P2 remain from this review; the one optional P3 compatibility-record cleanup is inline. Exact-head CI and Windows release checks passed.
中文
本轮精简已落实:撤回未来Session token机制,恢复WebSearch原契约,异常类回到client权威,附件同义code与重复文案合并。剥离rebase后的修复提交为+66/-148,没有剩余P0–P2。唯一可选P3是兼容声明收尾,详见行内,当前CI及Windows发布检查通过。
AI-assisted delta review by Codex; this refresh checks the simplification against the earlier reviewed behavior, not a new full UI acceptance run.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the revisions. I rechecked ba95e0823 specifically for solution size. The producer code → Electron transport → domain catalog direction is sound. Much of +1820/-632 is movement (WorkHub catalog +296 while the surface loses 256 lines), locale copy, and distinct IPC/pending/new-task regressions. I do not recommend a rewrite or a split solely to reduce this diff.
The inline P2 needs a small type import fix. An optional P3 simplification is to stop threading localized fallback text through policy.resolve and workHubNewSessionName: keep the surface→submit input, let policy return the extracted title, and apply decision.title || input.newSessionFallbackTitle once in the controller before workHub.act persists the proposal. Applying it only after returning to the surface would be too late. This removes presentation policy from routing without changing stored titles. The existing compatibility-record P3 still applies; the retired session-control tokens and other previously removed mechanisms should stay removed.
Please retain the serializable Plan envelope, attachment boundary checks, pending cleanup and real send regressions; they protect different production obligations.
中文
按当前 head 重新做了体量审计,主体方案合理,大量 diff 是文案搬运、三语映射和不同生产边界回归,不建议整体重写或为了行数拆 PR。行内 P2 只需补类型 import。可选精简是删除 policy 内 fallback 文案透传,在 controller 调 Host act 前统一补标题;不能等返回 surface 后才补,届时已持久化。保留必要 IPC envelope、附件边界校验与真实发送/pending 回归。
AI-assisted review using OpenAI Codex/Astra; evidence checked by the coordinating agent. This is not an independent human review.
a6cc0cc to
5446954
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for rebasing. At 544695401, the previous P2 is fixed: PlanControlIpcResult now has a real local type import in the bridge declaration. The producer-code → IPC-envelope → domain-presenter boundaries remain coherent; the rebase does not justify replacing them with a global mapper or removing the distinct pending/preflight regressions. No new P0–P2 finding in this pass.
Ten focused tests passed through the exact-head source loader, covering expected-error presentation, Plan pending behavior, and a nine-attachment new-task rejection before Session creation. Current-head test and Windows-package checks are green.
There is now a merge conflict in apps/desktop/renderer-architecture.json against main 8b3655a17. Please refresh the architecture ledger from the resolved source tree and rerun the existing ratchet. The synthetic merge's protocol declaration is correctly pinned to epoch 133. Actual Electron Plan success/failure/pending and localized attachment-error presentation remain outside these source probes; I am not renewing UI sign-off here.
AI-assisted independent review and primary source/integration/probe verification.
中文
感谢 rebase。544695401 已补上 PlanControlIpcResult 的本地类型 import,旧 P2 修复。错误码→IPC envelope→领域 presenter 的边界仍一致,无新 P0–P2,也没有依据改成全局 mapper 或删掉不同边界的回归。
exact-head 源码加载验证 10 项通过,覆盖错误呈现、Plan pending 和九附件发送在创建 Session 前拒绝;当前 test 和 Windows package 检查绿色。
目前与 main 8b3655a 的 renderer-architecture.json 冲突,请从解决后的源码刷新台账并重跑现有 ratchet。合成合并的协议声明 epoch 133 正确。实际 Electron Plan 成功/失败/pending 与本地化附件错误提示尚未通过本轮源码探针验收,因此这里不追加界面通过结论。经 AI 独立评审及主审源码/整合/探针核实。
b30c757 to
412922b
Compare
Raw exception text no longer reaches the UI: expected failures carry machine-readable codes (with params where needed) that the renderer maps through locale catalogs, and unexpected failures show a localized fallback while redacted diagnostics go to the console. Plan controls and work-board actions carry those codes across IPC in a structured envelope, since Electron strips custom fields off thrown Errors. Generated-by: Claude Code Generated-by: OpenCode Generated-by: pi (GLM)
412922b to
80dc732
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the update. Reviewed 80dc732 with an independent incremental reviewer and primary-agent reconciliation: no remaining P0–P2 findings. Error classification reuses the existing core classifier; diagnostics deduplicate the same error object. The proposal-exit channel retains its existing throwing contract: the previous preload adapter already discarded the envelope into an ordinary Error, and its only caller used the generic setting presenter. Removing that unused conversion does not regress an existing code-aware consumer.
Current-head test/package checks pass; the protocol guard passes on the synthetic merge with current main. Earlier focused source tests and the current CI provide regression evidence; no new local workspace suite or live Electron acceptance is claimed. This change does not redesign interaction, so the earlier broad manual Plan/attachment acceptance request is withdrawn. AI-assisted review.
中文
感谢更新。独立增量 reviewer 与主审复核当前 head,无剩余 P0–P2。错误分类复用既有 core classifier,同一个错误对象的诊断去重。退出 proposal 保留既有抛异常契约:旧 preload 本就把 envelope 转成普通 Error,唯一调用方使用通用设置提示;删除无人消费的转换不构成行为回退。
当前 test/package 通过,主审在当前 main 的合成合并上运行协议检查通过。回归依据是此前聚焦源码测试及当前 CI,不声称新增本地套件或真实 Electron 验收。本改动没有重新设计交互,撤回此前过宽的人工 Plan/附件验收要求。AI 辅助评审。
Summary
Desktop rendered raw exception text as user-facing copy. A zh user hitting an expected failure — a stale Work Board write, an import over the mount limit, a nine-attachment send — read an untranslated English
error.message, and unexpected exceptions leaked internal detail into toasts and error banners. The renderer could not tell the two apart, because failures crossed the process boundary as prose.Producers now return machine-readable codes instead of sentences, and each domain maps its own code union at its own presenter — there is no global error mapper. Expected failures carry a typed
ExpectedOperationError; everything else logs a redacted diagnostic and renders a localized fallback, both through one channel in@maka/core/redactionthat the copy catalogs and the renderer share. This deletes WorkHub's English-message-regex classifier and theMAKA_SESSION_READ_MESSAGES_ERRORmarker protocol.Two boundaries needed more than a code:
attachment_ingest:<code>token, andlocalizedShellErrorMessagematches it end-anchored, so a message that merely contains the substring (a path, say) keeps its fallback. Routing the renderer preflight through those tokens is what makes the new-task send path — which rejects before main's validation ever runs — report the real reason instead of a generic retry line.Errors, so a typed error can never reach the renderer as itself. The five plan-mode control channels return a structuredPlanControlIpcResultenvelope instead; the panel mapserror.codethroughplanModeCopy.controlFailure, and a rejection the envelope does not recognize is rethrown rather than reported as success.Between a stable code and a genuine defect sits a third case: a timeout, a 429, an auth rejection, a 5xx, a network fault. Those are categories, not defects, so they render their localized category and stay out of the diagnostics channel.
classifiedErrorFallbackin the renderer'soperation-diagnosticsentry is that rule, and every shell error-copy entry point now goes through it, so a command-palette action that times out reads "Request timed out" rather than a generic line plus a logged stack. One visible consequence:messageReadErrorMessageandmessageRefreshErrorMessageanswer a classifiable failure with its category instead of their contextual catalog line, matching whatmainalready did before the marker protocol was removed; the contextual line still covers everything the classifier does not recognize.The WorkHub waiting summary became a complete catalog message in each locale, independent of the separately displayed status and retry paragraphs, so a future translation can choose its own sentence structure.
The only protocol change is
plan.tsdeclaring the pre-existingunauthorizedcode on theplan.control/plan.turn.starterror specs and exportingPlanControlErrorCodeover the file-privateCONTROL_ERRORSlist, declared as a compatible extension at epoch 133. A structured{ok, code}envelope for the configuration setters is left as follow-up — those failures already carry protocol codes, so they are not part of the reachable gap this PR closes.Refs #2672
Verification
Before (zh UI — raw English internals, and an expected attachment failure lost to the generic fallback):
After (same three failures; the raw detail now only in the redacted console diagnostic):
AI use
Select exactly one:
Tool(s) and scope: three tools contributed under the contributor's direction, and the squashed commit carries a
Generated-bytrailer for each.Checklist